Skip to content
This repository was archived by the owner on Sep 6, 2021. It is now read-only.

add a tab with default extensions to extension manager - #13136

Merged
swmitra merged 8 commits into
masterfrom
zaggino/disable-default-extensions
Apr 13, 2017
Merged

swmitra merged 8 commits into
masterfrom
zaggino/disable-default-extensions

Conversation

@zaggino

@zaggino zaggino commented Mar 2, 2017

Copy link
Copy Markdown
Contributor

Adds a new tab that allows disabling/enabling default extensions (this functionality is already available, so this PR is mostly about displaying the list of default extensions).

Maybe we could use a different icon but I'm not capable of producing one and this can be replaced in a separate PR after this is merged.

Tagging @swmitra @redmunds to review as per discussion in #13108 but others feel free to chime in.

image

@ficristo

ficristo commented Mar 2, 2017

Copy link
Copy Markdown
Collaborator

I like how things come back #11618
I gave a try and seems I cannot disable the default Dark theme or JSLint.

@zaggino

zaggino commented Mar 2, 2017

Copy link
Copy Markdown
Contributor Author

Interesting, did you try it on Mac or Windows @ficristo ?

@zaggino

zaggino commented Mar 2, 2017

Copy link
Copy Markdown
Contributor Author

The thing it came back again probably means we should finish it this time 😄

@petetnt

petetnt commented Mar 2, 2017

Copy link
Copy Markdown
Collaborator

Nice @ficristo! I remembered that PR but couldn't find it myself.

👍 for the idea from me. Adding descriptions for the default extensions that don't have them would be nice.

@redmunds

redmunds commented Mar 2, 2017

Copy link
Copy Markdown
Contributor

I think disabling the Default Theme should not be allowed. Isn't that what gets used when you Restart Without Extensions ?

Also, JSLint extension is enabled by default. Maybe code needs to verify existence of linting module before trying to use it?

@zaggino

zaggino commented Mar 2, 2017 •

Copy link
Copy Markdown
Contributor Author

Disabling themes (as I agree it makes little sense) has been forbidden in 2d197fb

@zaggino
zaggino force-pushed the zaggino/disable-default-extensions branch from 8a3eab0 to 3d58d8e Compare March 2, 2017 23:48
@zaggino

zaggino commented Mar 2, 2017 •

Copy link
Copy Markdown
Contributor Author

Ok guys, did a few changes, please have a look. I've tested disabling/enabling JSLint and it works fine.
I've also tested disabling all the default extensions (except themes which now can't be disabled) and Brackets did startup just fine. I've added a warning to the console logs about the default extension being disabled as it might help with bug reports.

@petetnt adding descriptions would definitely be nice but I believe that can be handled as a starter issue.

@redmunds

redmunds commented Mar 3, 2017

Copy link
Copy Markdown
Contributor

@zaggino Built code, did minimal testing, and seems to work as advertised. Made a pass through code -- looks good. Awesome!

@ficristo

ficristo commented Mar 3, 2017

Copy link
Copy Markdown
Collaborator

I tryed again and JSLint is disabled by default and I cannot enabled. Is it normal or it is maybe related to my system?
In the preferences I have: "extensions.default.disabled": []

@zaggino

zaggino commented Mar 3, 2017

Copy link
Copy Markdown
Contributor Author

@ficristo if you have tried previous version, look at JSLint's directory if there's not a .disabled filename present and delete it or run git clean -fdx && npm install ... I had the same issue and this fixed it.

@ficristo

ficristo commented Mar 4, 2017

Copy link
Copy Markdown
Collaborator

Running git clean fixed it.
I wanted to disable JSLint only for Brackets so I added the preference to disable it in .brackets.json.
But JSLint is still available. Is this configuration not supported?

@zaggino

zaggino commented Mar 4, 2017

Copy link
Copy Markdown
Contributor Author

I don't think this could work per project @ficristo if that's what you're trying to do. The place where the extensions are being loaded does not consider which project you're in and neither does this get executed when you switch projects. While it wouldn't be hard to enable an extension when you switch to other project, it would be very difficult to disable one when you switch to a project which has it disabled in .brackets.json because disabling an extension basically means reloading whole editor. Now I know VSCode does that, maybe even Atom (reload whole editor when switching projects) but we currently don't and that'd be potentially huge change how Brackets work.

@zaggino

zaggino commented Mar 7, 2017

Copy link
Copy Markdown
Contributor Author

Any more comments @adobe/brackets-committers ?

@zaggino

zaggino commented Mar 12, 2017

Copy link
Copy Markdown
Contributor Author

@swmitra can anyone in your team review this please?

@zaggino zaggino added this to the Release 1.10 milestone Mar 23, 2017
@justinrusso

justinrusso commented Apr 3, 2017 •

Copy link
Copy Markdown
Contributor

With PR #13245 merged, the @extension-manager-min-width needs adjusting. Looks like 750px is good.

Note: This is located in src/styles/brackets_variables.less

@swmitra

swmitra commented Apr 5, 2017

Copy link
Copy Markdown
Collaborator

Very nice @zaggino 👍.
Tested this branch just now and everything seems to be working fine. I just have one small comment (But that would not block me to merge this PR now :smile), If we can cook some description for the default extensions to provide some meaningful info to the user based on which they can decide whether to disable any default extension!

@swmitra swmitra left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@swmitra

swmitra commented Apr 5, 2017

Copy link
Copy Markdown
Collaborator

@zaggino Can you please increase the min-width of the EM Dialog to ~760px?

@zaggino

zaggino commented Apr 13, 2017

Copy link
Copy Markdown
Contributor Author

@swmitra where's this min-width set?

I can see that EM has the width hard set to 760px here: https://github.com/adobe/brackets/blob/master/src/styles/brackets_patterns_override.less#L1026-L1029

@zaggino

zaggino commented Apr 13, 2017

Copy link
Copy Markdown
Contributor Author

I'll add descriptions to the default extensions in another PR, to prevent ongoing discussions what it should be :)

@zaggino

zaggino commented Apr 13, 2017

Copy link
Copy Markdown
Contributor Author

ah, found it @swmitra @justinrusso please check if d6cf7c0 is correct

@swmitra

swmitra commented Apr 13, 2017

Copy link
Copy Markdown
Collaborator

@zaggino Yes that's the place. I was about to reply but you are fast 😄 I have tested this feature and works as expected. Merging ...

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants